Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[full-ci] Acceptance tests for deep moves and fix 39702 #39703

Merged
merged 3 commits into from
Jan 21, 2022
Merged

[full-ci] Acceptance tests for deep moves and fix 39702 #39703

merged 3 commits into from
Jan 21, 2022

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Jan 20, 2022 •
edited
Loading

Description

  1. add test scenarios for combinations of moving a file between folders where the folder and/or file names are numeric
  2. Update sabre/dav (4.3.0 to 4.3.1), which fixes the issue
  3. Add test scenarios for moving file from subfolder to root

Related Issue

fixes #39702

How Has This Been Tested?

CI

In the first commit: https://drone.owncloud.com/owncloud/core/34371/93/13

runsh: Total unexpected failed scenarios throughout the test run:
apiWebdavMove2/moveFile.feature:421

In the 3rd commit, locally without tghe fix, I get:

  Background:                                                                            # /home/phil/git/owncloud/core/tests/acceptance/features/apiWebdavMove2/moveFile.feature:7
    Given using OCS API version "1"                                                      # FeatureContext::usingOcsApiVersion()
    And user "Alice" has been created with default attributes and without skeleton files # FeatureContext::userHasBeenCreatedWithDefaultAttributesAndWithoutSkeletonFiles()

  Scenario Outline: Moving a file from a folder to the root                                                         # /home/phil/git/owncloud/core/tests/acceptance/features/apiWebdavMove2/moveFile.feature:437
    Given using <dav_version> DAV path                                                                              # FeatureContext::usingOldOrNewDavPath()
    And user "Alice" has created folder "<source_folder>"                                                           # FeatureContext::userHasCreatedFolder()
    And user "Alice" has uploaded file with content "ownCloud test text file 0" to "/<source_folder>/<source_file>" # FeatureContext::userHasUploadedAFileWithContentTo()
    When user "Alice" moves file "/<source_folder>/<source_file>" to "/<target_file>" using the WebDAV API          # FeatureContext::userMovesFileUsingTheAPI()
    Then the HTTP status code should be "201"                                                                       # FeatureContext::thenTheHTTPStatusCodeShouldBe()
    And the following headers should match these regular expressions for user "Alice"                               # FeatureContext::headersShouldMatchRegularExpressionsForUser()
      | ETag | /^"[a-f0-9:\.]{1,32}"$/ |
    And the content of file "/<target_file>" for user "Alice" should be "ownCloud test text file 0"                 # FeatureContext::contentOfFileForUserShouldBe()

    Examples:
      | dav_version | source_folder | source_file | target_file |
      | old         | 0             | file.txt    | file.txt    |
      | old         | 1             | file.txt    | file.txt    |
      | old         | texta         | 0           | file.txt    |
      | old         | texta         | 1           | file.txt    |
      | old         | texta         | file.txt    | 0           |
      | old         | texta         | file.txt    | 1           |
        HTTP status code 500 is not the expected value 201
        Failed asserting that 500 matches expected '201'.
      | new         | 0             | file.txt    | file.txt    |
      | new         | 1             | file.txt    | file.txt    |
      | new         | texta         | 0           | file.txt    |
      | new         | texta         | 1           | file.txt    |
      | new         | texta         | file.txt    | 0           |
      | new         | texta         | file.txt    | 1           |

--- Failed scenarios:

    /home/phil/git/owncloud/core/tests/acceptance/features/apiWebdavMove2/moveFile.feature:453

12 scenarios (11 passed, 1 failed)
108 steps (105 passed, 1 failed, 2 skipped)
1m51.32s (18.36Mb)

The fails happened when the target of the move was a path that starts with a non-zero number
For example, moving a file up to the root and calling it "1".
Or moving a file to a folder called "1".

Somehow the cases where the target path starts with a "0" did not fail.

CI passes with the 2nd commit, and 3rd commit.

Also, everything was OK with the "new" dav_version. Only "old" dav_version failed. That seems strange, but I guess there must be different ways that those make use of sabre/dav

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@phil-davis phil-davis self-assigned this Jan 20, 2022
This was referenced Jan 20, 2022
Closed
Merged
@phil-davis phil-davis marked this pull request as ready for review January 20, 2022 14:36
@phil-davis phil-davis added dependencies php Pull requests that update Php code labels Jan 20, 2022
@phil-davis phil-davis changed the title [tests-only] [full-ci] Acceptance tests for deep moves [full-ci] Acceptance tests for deep moves and fix 39702 Jan 20, 2022
individual-it
individual-it reviewed Jan 20, 2022
View reviewed changes
Copy link
Member

@individual-it individual-it left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this issue was first discovered using the windows explorer and I it only happened when moving a numeric file/folder to the root of the tree
so you maybe want to add those tests also

@phil-davis
Copy link
Contributor Author

this issue was first discovered using the windows explorer and I it only happened when moving a numeric file/folder to the root of the tree so you maybe want to add those tests also

done in 3rd commit

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@phil-davis phil-davis merged commit 65b2cfc into master Jan 21, 2022
@delete-merged-branch delete-merged-branch bot deleted the issue-39702 branch January 21, 2022 07:48
@phil-davis phil-davis mentioned this pull request Jan 24, 2022
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@individual-it individual-it individual-it approved these changes

@jvillafanez jvillafanez jvillafanez approved these changes

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

@IljaN IljaN Awaiting requested review from IljaN

@JammingBen JammingBen Awaiting requested review from JammingBen

@AlexAndBear AlexAndBear Awaiting requested review from AlexAndBear

Assignees

@phil-davis phil-davis

Labels
dependencies php Pull requests that update Php code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Moving a file into a folder that is a number fails
3 participants
@phil-davis @jvillafanez @individual-it